Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid duplicate file level functions and variables in namespace rewrite #918

Merged
merged 22 commits into from
Nov 28, 2023

Conversation

Amxx
Copy link
Contributor

@Amxx Amxx commented Nov 8, 2023

To extract storage layouts for namespaces, the Hardhat plugin makes a copy of the user's source code and makes modifications to this copy: it injects storage variables for namespaced structs and removes everything that is not needed (to speed up compilation).

File level functions and variables are not removed, but are replaced with dummy constants in case they are imported by another file. However, overloaded functions would cause duplicate identifiers to exist, which causes compilation to fail when compiling this modified copy of source code.

We should keep track of identifiers to avoid duplicates in this case.

Fixes #917

@ericglau ericglau changed the title Remove file level functions and variables from namespace rewrite Avoid duplicate file level functions and variables in namespace rewrite Nov 24, 2023
@ericglau
Copy link
Member

ericglau commented Nov 24, 2023

@Amxx I changed this to keep dummy variables for ErrorDefinition, FunctionDefinition, and VariableDeclaration, but avoids duplicate identifiers if one already exists due to overloading. For now I think FunctionDefinition is really the only case where overloading could occur, but it could happen for ErrorDefinition when Solidity eventually supports overloading custom errors. So for simplicity, I use the same code for all 3 of these cases.

We now also replace UsingForDirective with dummy enums to avoid orphaning their NatSpec. The NatSpec of UsingForDirective is not part of the AST so we cannot easily delete those.

Can you please review the overall changes?

Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just minor question

packages/core/src/utils/make-namespaced.ts Outdated Show resolved Hide resolved
@ericglau
Copy link
Member

Summary:

Changed to not delete any nodes and not delete documentation. Instead, replaces functions and other unneeded nodes with dummy enums, either with the original name (if the identifier could be imported in other files) or with a random identifier based on the AST node ID (if the identifier is no longer relevant or if it was originally an overloaded function).

This avoids any possibility of NatSpec getting orphaned or mixed up with different nodes than originally written, which could lead to compile errors. Enums are able to compile with any NatSpec tag even if they are not relevant.

@ericglau ericglau merged commit 5da44b1 into OpenZeppelin:master Nov 28, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hardhat compile error with @openzeppelin/hardhat-upgrades
3 participants